Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add stream_socket_test stream_write_test #231

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

YoshiakiNishimura
Copy link
Contributor

Test for 365d48e (Improve TCP connection by buffering data for faster writes)

@YoshiakiNishimura YoshiakiNishimura marked this pull request as ready for review August 22, 2024 10:24
EXPECT_EQ(sw->write(data.c_str(), 0), tateyama::status::ok);
EXPECT_EQ(sw->write(nullptr, 0), tateyama::status::ok);
EXPECT_EQ(sw->write(nullptr, INT_MIN), tateyama::status::ok);
EXPECT_EQ(sw->write(nullptr, INT_MAX), tateyama::status::ok);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

このあたりのテストでstatus::okが戻るのは自明なので、できればcommitをするまでバッファに書かれない、commitでバッファされていたものが書かれる、というところが確認できるといいですね。ただ、既存のコードに変更が必要になるかもしれないので最終的にどこまでテストするかは @t-horikawa さんと相談してみてください。

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

buffer_がprivate member variableなので

  • buffer_ へのアクセス関数を用意
  • フレンドクラスを使用する

の2択ですが、これはテストコード以外を修正する必要があるのでいったんこのテストコードをmergeして別のcommitとしてよいですか?

@kuron99 @t-horikawa アクセス関数とフレンドクラスの場合どちらがtsurugiの流儀になりますか?指定がなければsetterを用意してbuffer_を編集して、同時にgetterを用意してbuffer_の中身の確認するテストも追加します。

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tsurugiの流儀と言えるものはない気がするのですが、私がやるとすればテスト用のアクセス関数を追加するほうですね。カプセル化を壊すのですが、コメントでテスト専用と書いて、本番コードからは呼ばないように気をつけると。
あとテストのやり方によってはgetterだけあればOKかもしれません。

Copy link
Contributor

@kuron99 kuron99 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@t-horikawa さんのapprovalもらったらマージしてOKです。

@t-horikawa
Copy link
Contributor

すいません。何をテストしようとしているのか良く分からないです。
「tateyamaのcode coverageが低いのでそれを上げようとしている」は、大方針としてはOK.
で、テスト対象(code coverageが低い部分)は、stream.hでしょうか?それとも、stream_response.[cpp|h]?
前者だとすると、stream.hはサーバ側で使う(connection_socketでaccept()したsocketをstream_socketが受け取る)ことを想定していますが、そのままではテストが難しそうです。
確かに作成されたtestのように、socket()で作成したsocketを渡すのも一手。ただ、この状態だと、sendやwriteで書き込んだデータはどこにも送っていない(socket(AF_INET, SOCK_STREAM, 0)しただけのsocketなので)ですよね。その状態で、tateyama::status::okが返ってくる、というのは(現実装ではそうなっていますが、本来、send()ではエラーになるはずなので、)stream_socketの実装が甘いことによるもので、将来、それが改善されたらこのテストは通らなくなります。
他のtest/tateyama/endpoint/streamにあるテストだと、stream_clientを用意し、TCP/IP接続を行ったうえでデータ転送しています。stream_socketに渡すsocketはaccept()したものでなく、(connect()する)クライアント側のsocketでも良いのですが、実際にデータ通信を行い、所定のデータが相手に届くのを確認する、というテストにする必要があるのではと思います。

なので、マージはpendingとさせてください。改良案は私も考えます。

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants